Got it, will do benchmark test then.

Test with my own machine and it shows parallel mode is 20%-50% faster than single mode while the number of nodes is below 5k, see the picture below.

Hardware overview:
Total Number of Cores: 10 (8 performance and 2 efficiency)
Memory: 32 GB

Benchmark results on my linux machine

goos: linux
goarch: amd64
pkg: github.com/ethereum/go-ethereum/trie
cpu: Intel(R) Core(TM) i7-14700K
BenchmarkCommit/commit-100nodes-single-28                  18546             66232 ns/op          115191 B/op       1462 allocs/op
BenchmarkCommit/commit-100nodes-parallel-28                18679             65397 ns/op          115205 B/op       1462 allocs/op
BenchmarkCommit/commit-200nodes-single-28                  10000            124273 ns/op          233517 B/op       2937 allocs/op
BenchmarkCommit/commit-200nodes-parallel-28                14197             92260 ns/op          235108 B/op       2968 allocs/op
BenchmarkCommit/commit-500nodes-single-28                   3777            313145 ns/op          604465 B/op       7606 allocs/op
BenchmarkCommit/commit-500nodes-parallel-28                 6948            336104 ns/op          606177 B/op       7638 allocs/op
BenchmarkCommit/commit-1000nodes-single-28                  1622            623420 ns/op         1174285 B/op      14816 allocs/op
BenchmarkCommit/commit-1000nodes-parallel-28                4302            320331 ns/op         1176365 B/op      14853 allocs/op
BenchmarkCommit/commit-2000nodes-single-28                  1098           1221359 ns/op         2305265 B/op      28850 allocs/op
BenchmarkCommit/commit-2000nodes-parallel-28                2102            600721 ns/op         2306553 B/op      28878 allocs/op
BenchmarkCommit/commit-5000nodes-single-28                   392           3530741 ns/op         6472302 B/op      74933 allocs/op
BenchmarkCommit/commit-5000nodes-parallel-28                 862           1615425 ns/op         6469649 B/op      74926 allocs/op
PASS

Deployed the PR/Master on benchmark 07/08 for one hour, it turns out this pull request does improve the account trie commit a bit.

Deployed the PR/Master on benchmark 07/08 for one hour, it turns out this pull request does improve the account trie commit a bit.

I also looked at those charts a bit. I'm a bit confused. Where is snapshot account read, and snapshot storage read? I guess we have not updated both side-by-side charts, only one, after changing in the metrics?

Also relevant (this PR starts about halfway in, time-wise)

@holiman Yes, i just deployed it on a ongoing benchmark pairs (new release vs last release) to have a quick test.

I also looked at those charts a bit. I'm a bit confused. Where is snapshot account read, and snapshot storage read?

snapshot account read and snapshot storage read metrics are deleted in state reader abstraction PR. Now you have to use account read and storage read instead.

we have not updated both side-by-side charts, only one, after changing in the metrics

True, we need to update them

I've attempted to do it differently.

Concentrating only on the larg(er) trie, here are my numbers for this PR:

BenchmarkCommit/commit-5000nodes-single-8                      110           13174997 ns/op         6467652 B/op      74881 allocs/op
BenchmarkCommit/commit-5000nodes-parallel-8                    212            5879169 ns/op         6471346 B/op      74935 allocs/op

And the unmodified master, with a similar benchmark:

// BenchmarkCommit/commit-5000nodes-single-8         	       144      	   9601554 ns/op	     4070863 B/op	   51615 allocs/op

Thus: for non-parallel, this PR is a degradation, most likely because of the additional mem usage, which goes up by ~50%. So speed goes from 9ms -> 13ms whereas the parallel makes it go from 9ms to 6ms.

The memory overhead is what I saw in the graphs above, on the live benchmark nodes.

In my branch parallel_commit_alt, I've attempted a different approach: branch against upstream master, diff against your branch

• Get rid of the wrapped node struct
• On top level, spin out new committers, one per goroutine. Each committer has it's own internal nodeset. After finishing it's work, each goroutine merges it's own nodeset with the parent nodeset.

With that approach, I get

BenchmarkCommit/commit-5000nodes-single-8                    122          10880174 ns/op         4623681 B/op      55263 allocs/op
BenchmarkCommit/commit-5000nodes-parallel-8                  331           3701590 ns/op         4726417 B/op      55604 allocs/op

So the 9ms -> 11ms for single-thread (but I suspect that might be a fluke), and 4ms for multi-threaded approach. The mem usage is only marginally higher than current code.

OBS: I have not ascertained that the implementation actually is correct: that the nodes collected are the right ones. So the next step would be to actually test that it does what it's supposed to do. But all in all, I don't think the current +50% mem usage is a workable approach.

Fixed up now so it is correct. New numbers on my branch (basically no change):

BenchmarkCommit/commit-5000nodes-single-8                    122          11983636 ns/op         4623249 B/op      55258 allocs/op
BenchmarkCommit/commit-5000nodes-parallel-8                  261           4930507 ns/op         5592596 B/op      55802 allocs/op

Closing this in favour of #30545 . Thanks for picking it up from the start!